Adds more complex focus control for DataGrid #2222

myasonik · 2019-08-13T14:30:27Z

Summary

Improved focus management for more complex cell contents. From the a11y spec, this covers:

if a cell has one focusable element, default focus can go directly to the element
if a cell has more than one focusable element or 1 focusable element with other content, the cell should receive focus but make it clear there is interactive content
on [enter] if a cell contains one or more widgets, disables grid navigation and places focus on the first widget
on [F2] same as enter; pressing [F2] again restores grid navigation
[esc] restore grid navigation; if content was being edited, it may undo/cancel
if grid navigation is disabled, [shift+tab/tab] can now navigate through widgets inside a cell which is now a focus trap

Checklist

- [ ] Checked in dark mode
- [ ] Checked in mobile

Checked in IE11
Checked in Firefox
- [ ] Props have proper autodocs
~~- [ ] Added documentation examples~~
Added or updated jest tests
~~- [ ] Checked for breaking changes and labeled appropriately~~
Checked for accessibility including keyboard-only and screenreader modes
~~- [ ] A changelog entry exists and is marked appropriately~~

myasonik · 2019-08-13T14:32:18Z

@thompsongl - could you verify my approach here before I spend time writing tests for it all? I know I dipped into DOM API quite a bit here but I wasn't sure how else to solve some of these focus management problems while keeping the cell contents completely open.

thompsongl · 2019-08-13T18:29:22Z

@myasonik The approach looks sane. I understand the places you used the DOM API, and just one stuck out as potentially having a simpler solution.
Where CELL_CONTENTS_ATTR is queried at the data_grid level to determine if the target cell has tabbable content (isInteractiveCell()): Is it possible to have the data_grid_cell set a flag on that attribute to describe whether the cell has tabbable content? All necessary information (I think) is already available in updateFocus(), and the same childNodes check takes place there. It would avoid duplicate logic and remove some responsibility from data_grid.
Other than that, I agree with the choice to not maintain a register of all cells and their contents at the data_grid level.

One point of discrepancy: In the docs/example, focusing a cell in the avatar_url column places focuses the cell itself, while focusing a cell in the a bug column places focus directly on the icon button. In both cases, the cell only has a single focusable child element, but results in different interaction. I saw a TODO about fixing the bug column. Is this a known thing?

myasonik · 2019-08-16T12:37:59Z

So, it seems the tabbable library just does not work in tests... I'm not sure what to do at this point so I've just skipped the tests I intended to write.

Any thoughts from the team on how to proceed? I see either:

don't worry about the tests for focus management
contribute back to the tabbable library (they're looking for co-maintainers so we could take that mantle on)
try to write some sort of test shim for it

I think 3 is the worst option. 1 is the easiest but 2 is probably the best. (But, I'm not sure how much work it might be. I didn't really dig into it too much.)

EDIT: Seems like it used to work in tests but latest version doesn't... Will look into it... 🤔 😢

thompsongl · 2019-08-16T14:38:10Z

@myasonik I didn't see til just now that the tabbable version bumped. Bummer if it's less testable.

Chandler may have ideas next week.

myasonik · 2019-08-20T13:26:38Z

package.json

@@ -61,7 +61,7 @@
    "react-is": "~16.3.0",
    "react-virtualized": "^9.18.5",
    "resize-observer-polyfill": "^1.5.0",
-    "tabbable": "^1.1.0",
+    "tabbable": "^3.0.0",


Upgrading to 4.x breaks all existing tests that rely on it

Was this using 4.x when we last talked and nothing was working?

src/components/datagrid/data_grid.test.tsx

src/components/datagrid/data_grid.tsx

chandlerprall · 2019-08-22T15:02:34Z

src/components/datagrid/data_grid.tsx

+      return false;
+    }
+
+    const cellContents = element.querySelector(`[${CELL_CONTENTS_ATTR}]`);


this line through the end of the function can be simplified to

return !!element.querySelector([${CELL_CONTENTS_ATTR}="true"])

Thoughts on using Boolean() instead of !!? I just think it's a lot clearer of what's going on that way.

!! is a common pattern in JavaScript, so I'm not too concerned on readability here. When that is an issue I'd favor changing to

return element.querySelector([${CELL_CONTENTS_ATTR}="true"]) != null

which returns the result of the comparison instead of abusing !! to cast. I avoid capital-letter Number() and Boolean() calls because I automtically throw the new keyword in front of them which returns an object version and fails an equality check

true === new Boolean(true) // false true === Boolean(true) // true

it's all personal preference, so honestly whatever you want to use here

src/components/datagrid/data_grid.tsx

src/components/datagrid/data_grid_cell.tsx

chandlerprall

Changes LGTM; pulled and tested locally

Only issue I see is:

arrow-key to a cell that auto-focuses its single item
tab out of the grid
tab back into the grid, the cell has focus instead of its content

thompsongl · 2019-08-28T14:27:10Z

Same as what @chandlerprall said, but with the addition of

shift+tab out of a cell that has auto-focused its single item puts the focus on the cell, not outside of the grid

* initial datagrid * protect against re-rendering content on column width change * Added unit initial tests for EuiDataGrid * [EuiDataGrid] Base layer Sass (#2171) * Initial css fixes * works in IE, addresses feedback * remove user select * Adds basic aria roles and grid navigation (#2187) * Adds basic aria roles and grid navigation Co-Authored-By: Chandler Prall <chandler.prall@gmail.com> * [Data Grid] Grid style options (#2176) Adds basic styling to the data grid. * Add pagination to EuiDataGrid (#2188) * Added pagination to to EuiDataGrid * Move EuiDataGrid row rendering to a sub-component to clean up state management * EuiDataGrid pagination unit tests * fix data grid pagination * revert colors * EuiDataGrid column visibility & ordering (#2207) * Show/hide and re-order datagrid columns * Column visability & ordering tests * column styling * column sizing and bars * blergh * tests * feedback * Fix linting * Adds more complex focus control for DataGrid (#2222) * [Data Grid] - Styling built into data grid, full screen mode (#2230) Styling built into data grid, full screen mode * EuiDataGrid add column sorting (#2278) * API interface for providing column sort order, callback to allow external data sorting * EuiDataGrid renders content into memory, sorts on it * Added tests for EuiDataGrid sorting * Added aria-sort value to a singly-sorted column header * small cleanup * add tests back in, though they are still broken * Clean up some keyboard navigation issues * Fix column sorting & update snapshots * EuiDataGrid hooks cleanup (#2331) * Refactored EuiDataGrid's hooks * Fix datagrid to react to gridStyle changes * [EuiDataGrid] Automatic column schema detection (#2351) * Automatically detect data schema for in-memory datagrid * Merge in described schema for field formatting * Better column type detection * Tests for euidatagrid schema / column type * refactor datagrid schema code, add datetime type detection * some comments * Allow extra type detectors for EuiDataGrid * cleanup of docs and type formatting * Fix datagrid unit test * Update currency detector * Allow EuiDataGrid's inMemory prop to be {true} * Added ability to provide extra props for the containing cell div * Added test for cell props * [EuiDataGrid] InMemory Performance (#2374) * Automatically detect data schema for in-memory datagrid * Merge in described schema for field formatting * Better column type detection * Tests for euidatagrid schema / column type * refactor datagrid schema code, add datetime type detection * some comments * Allow extra type detectors for EuiDataGrid * cleanup of docs and type formatting * Fix datagrid unit test * Update currency detector * Allow EuiDataGrid's inMemory prop to be {true} * Added ability to provide extra props for the containing cell div * Added test for cell props * Performance cleanups * Clean up datagrid doc's inMemory selection * Merged in feature branch * EuiDataGrid in-memory options * Performance refactor for in-memory values * added a comment * Fix sorting on in-memory and schema datagrid docs * [EuiDataGrid] Feature/euidatagrid menu ux (#2392) Moved the sorting mechanism to the top toolbar. * Export useRenderToText to top-level package (#2412) * [DATA GRID] Expand cells (#2418) Data grid cells now can expand and can render individually based upon their schema. * [EuiDataGrid] use schema information when sorting (#2419) * cell expansion working mostly * fix double import * add search to field selector * euitext * cell epansion is now optional through a config * keydown event for cells * remove tabbables * Clean up some code & tests * Remove unused line of code * Center popover against cell * Update euidatagridcell popover placement, trigger, dom structure, and auto focusing * Restore focus to grid cell when popover was in response to mouse click * Allow grid column selection to be searchable * Refactor expansion popover formatting, allow custom ones * schema-based sort comparators * reverse boolean sort to be true-false * adds json schema sorting, fixes issue with popover * Weaken the currency type detector when values have a period in their first few characters, and fix test * Move column order and visibility to be managed externally (#2422) * fix tests * [EuiDataGrid] Custom column headers (#2421) * Allow custom ReactNode for column header display * Allow navigation into grid headers if any are interactive * Properly wrap cell focus and use [enter], [f2] to interact * Corrected header cell focus-state on blurring, [escape]. and single interactives * Corrected header cell focus-state on blurring, [escape]. and single interactives * When datagrid header is interactive, default its tabstop to the first header cell * EuiDataGridHeaderCell warns about multiple interactive elements * fix focus, example and screenreader stuffs, looks like tests pass * simplifying screen reader read out * [DATA GRID] EuiGridToolBar toolbar is now configurable through props (#2443) * EuiGridToolBar toolbar is now configurable through props * better tests * small test typp * Update src/components/datagrid/data_grid_types.ts Co-Authored-By: Greg Thompson <thompsongl@users.noreply.github.com> * feedback * [EuiDataGrid] Docs and autodocs (#2449) * Render out EuiDataGrid proptypes * Add pagination props to docs * Fill out all datagrid autodoc sections * remove debugger statement * Update src/components/datagrid/data_grid_types.ts Co-Authored-By: Greg Thompson <thompsongl@users.noreply.github.com> * words * docs start * datatype renamed to schema, update docs * docs, fix typo for fullscreen buton * core concepts * better in memory explanation * custom schema example * provide a nice, documented snippet * typos * don't show pagination when only one page * clean up styling, better docs for formatters * more docs cleanup * IE fix * IE fix again * small cleanup of docs * describe how to disable expansion popovers * dark mode tweaks * Fix custom datatype sorting * Update src-docs/src/views/datagrid/datagrid_example.js Co-Authored-By: Michail Yasonik <michail@yasonik.com> * Update src-docs/src/views/datagrid/datagrid_example.js Co-Authored-By: Michail Yasonik <michail@yasonik.com> * Update src-docs/src/views/datagrid/datagrid_example.js Co-Authored-By: Michail Yasonik <michail@yasonik.com> * Update src-docs/src/views/datagrid/datagrid_example.js Co-Authored-By: Michail Yasonik <michail@yasonik.com> * Update src-docs/src/views/datagrid/datagrid_example.js Co-Authored-By: Michail Yasonik <michail@yasonik.com> * Update src-docs/src/views/datagrid/datagrid_example.js Co-Authored-By: Michail Yasonik <michail@yasonik.com> * Update src-docs/src/views/datagrid/datagrid_example.js Co-Authored-By: Michail Yasonik <michail@yasonik.com> * Update src-docs/src/views/datagrid/datagrid_example.js Co-Authored-By: Michail Yasonik <michail@yasonik.com> * PR feedback * typo * feedback to break up docs * better cross linking and summary * fix custom schema display * Update src-docs/src/views/datagrid/datagrid_memory_example.js Co-Authored-By: Greg Thompson <thompsongl@users.noreply.github.com> * Update src-docs/src/views/datagrid/datagrid_memory_example.js Co-Authored-By: Greg Thompson <thompsongl@users.noreply.github.com> * Update src-docs/src/views/datagrid/datagrid_memory_example.js Co-Authored-By: Greg Thompson <thompsongl@users.noreply.github.com> * Update src-docs/src/views/datagrid/datagrid_schema_example.js Co-Authored-By: Greg Thompson <thompsongl@users.noreply.github.com> * Update src-docs/src/views/datagrid/datagrid_memory_example.js Co-Authored-By: Greg Thompson <thompsongl@users.noreply.github.com> * Update src-docs/src/views/datagrid/datagrid_memory_example.js Co-Authored-By: Greg Thompson <thompsongl@users.noreply.github.com> * Update src-docs/src/views/datagrid/datagrid_memory_example.js Co-Authored-By: Greg Thompson <thompsongl@users.noreply.github.com> * Updated some datagrid docs * main dg example page feedback * Eui prefix all the things to be consistant. Adjust the data grid docs to match * rewrite intro based on feedback * more tweaking of words * rename toolbarDisplay->toolbarVisibility * in memory docs reworked to four examples * clean up core example * data grid styling snippets * fix prop list * Minor grammar edits * Added isDetails prop to renderCellValue, reducing the use case for expansionFormatters. Speaking of those, expansionFormatter(s) has been renamed to popoverContent(s) and now recieve the rendered cell div in addition to the renderCellValue ReactElement * fix docs renaming, fix css * last docs edit seems fitting * somewhat decent attempt at putting classnames on schemas * Revert "somewhat decent attempt at putting classnames on schemas" This reverts commit 26542d7. * changelog

myasonik requested a review from thompsongl August 13, 2019 14:30

myasonik force-pushed the advanced-focus branch from 20b56fc to 8807d33 Compare August 13, 2019 14:33

myasonik requested review from chandlerprall and snide August 16, 2019 12:34

myasonik marked this pull request as ready for review August 16, 2019 12:38

myasonik force-pushed the advanced-focus branch from 64b67d0 to b7e9723 Compare August 16, 2019 12:52

myasonik commented Aug 20, 2019

View reviewed changes

chandlerprall suggested changes Aug 22, 2019

View reviewed changes

myasonik requested a review from chandlerprall August 23, 2019 16:18

Adds more complex focus control for DataGrid

8b1f70f

myasonik force-pushed the advanced-focus branch from 2950fac to 8b1f70f Compare August 26, 2019 16:43

chandlerprall suggested changes Aug 27, 2019

View reviewed changes

myasonik merged commit 1bd21d8 into elastic:feature/euidatagrid Aug 29, 2019

myasonik deleted the advanced-focus branch August 29, 2019 15:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds more complex focus control for DataGrid #2222

Adds more complex focus control for DataGrid #2222

myasonik commented Aug 13, 2019 •

edited

Loading

myasonik commented Aug 13, 2019

thompsongl commented Aug 13, 2019

myasonik commented Aug 16, 2019 •

edited

Loading

thompsongl commented Aug 16, 2019

myasonik Aug 20, 2019

thompsongl Aug 28, 2019

chandlerprall Aug 22, 2019

myasonik Aug 23, 2019

chandlerprall Aug 27, 2019

chandlerprall left a comment

thompsongl commented Aug 28, 2019

Adds more complex focus control for DataGrid #2222

Adds more complex focus control for DataGrid #2222

Conversation

myasonik commented Aug 13, 2019 • edited Loading

Summary

Checklist

myasonik commented Aug 13, 2019

thompsongl commented Aug 13, 2019

myasonik commented Aug 16, 2019 • edited Loading

thompsongl commented Aug 16, 2019

myasonik Aug 20, 2019

Choose a reason for hiding this comment

thompsongl Aug 28, 2019

Choose a reason for hiding this comment

chandlerprall Aug 22, 2019

Choose a reason for hiding this comment

myasonik Aug 23, 2019

Choose a reason for hiding this comment

chandlerprall Aug 27, 2019

Choose a reason for hiding this comment

chandlerprall left a comment

Choose a reason for hiding this comment

thompsongl commented Aug 28, 2019

myasonik commented Aug 13, 2019 •

edited

Loading

myasonik commented Aug 16, 2019 •

edited

Loading